8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

koppor · 2019-12-21T15:36:14Z

This is a WIP PR. Requesting for comments.

I could not replicate the test given at https://bugs.openjdk.java.net/browse/JDK-8176270 without TestFX. I nevertheless let my try to replicate the @test things in here.

The fix is just a wild guess without really understanding the side effects of .addListener.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

Download

$ git fetch https://git.openjdk.java.net/jfx pull/73/head:pull/73
$ git checkout pull/73

bridgekeeper · 2019-12-21T15:36:47Z

Hi koppor, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user koppor" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

koppor · 2019-12-21T15:38:22Z

/signed

bridgekeeper · 2019-12-21T15:38:50Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

mlbridge · 2020-02-07T14:33:51Z

Webrevs

00: Full (492cb6c)

kevinrushforth

I left comments inline. This will need changes. Also, you will need to provide a test that fails without the fix and passes with the fix.

kevinrushforth · 2020-02-07T18:04:35Z

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java

-                if (end > start + length) end = length;
-                if (start > length-1) start = end = 0;
+                // Ensure that the last character to get is within the bounds of the txt string
+                if (end >= start + length) end = length-1;


First, I think the existing test is simply wrong: Why should the start value matter when checking whether the end value needs to be clamped -- it's an end point not a number of characters. I think the entire problem might be the fact that it is comparing against start + length. Second, the value to be clamped to was correct before your change. The substring method to which end is passed is documented as subtracting 1.

I think the test should be something like:

if (end > length) end = length;

on second thought: correct clamping or not, the start/end indices of selection are invalid whenever text was removed/added at an index before the selection - they are in coordinates of the old text, not the new. If they point to an index > new textLength, incorrect clamping will throw. If they are both < new textLength, it'll fire an intermediate changeEvent with incorrect selection, the total sequence beining
oldSelectedText -> incorrectly-shifted-selectedText
incorrectly-shifted-selectedText -> empty

The correct change notification would be, because at the end of the text change, the selection is cleared,
oldSelectedText -> empty

To me, it looks like using a binding here is not the best of options.

kevinrushforth · 2020-02-07T18:12:23Z

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java

+                // Ensure that the last character to get is within the bounds of the txt string
+                if (end >= start + length) end = length-1;
+                // In case the start is after the whole txt, nothing valid is selected. Thus, return the default.
+                if (start >= length) return "";


This change seems OK, and might be clearer than the existing code, but the existing code is correct, and produces the same effect.

yeah, it is correct - but while we are changing the impl, we might go the whole way and clean up :) my pref would be to add the check for start to the short-circuit at the beginning of the method, something like

String txt = text.get(); IndexRange sel = selection.get(); if (txt == null || sel == null || sel.getStart() >= text.getLength()) return "";

kevinrushforth · 2020-02-07T18:23:09Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

+                 });
+        textField.positionCaret(5);
+        Semaphore semaphore = new Semaphore(0);
+        Platform.runLater(semaphore::release);


Since controls tests run using the StubToolkit, Platform.runLater does not do what you expect. If you need to run a pulse to get some code to run that normally would run as part of pulse, you can call that method directly. See other controls tests for how this is done.

kleopatra · 2020-02-09T14:40:25Z

Just a comment on how to add a failing test: we can replace the actual typing (in the TestFx context) by programmatically replacing the selection, actually, it doesn't even need the skin (so no scene nor parent) because all collaborators are in TextInputControl:

  /** 
    * Test for JDK-8176270: register changeListener on selectedText, select at
    * end of text and replace selection throws.
    */
@Test 
public void addingChangeListenerNoSkin() {
    TextField textField = new TextField();
    textField.setText("1234 5678");
    textField.selectedTextProperty()
             .addListener((o, ov, nv) -> {});

    textField.positionCaret(5);
    textField.selectEnd();
    textField.replaceSelection("d");
}

Similarly, an invalidationListener that access the selectedText throws, while an invalidationListener not accessing the selected is fine. ~~So it looks like a one-off when evaluating the actual selection during the process, somehow it's not yet ready~~ That was my wrong assumption, the culprit is really the incorrect clamping of a no-longer valid end index when adjusting the selectedText binding (as @kevin noted above). It shows only if the selectedText must be re-evaluated before the selectionRange is updated, f.i. forced by a changeListener (or direct access).

To make the error show up on the test thread, replace the uncaughtExceptionHandler before (and cleanup after):

@Before public void setup() throws Exception {
    Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
        if (throwable instanceof RuntimeException) {
            throw (RuntimeException)throwable;
        } else {
            Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
        }
    });
    textInput = (TextInputControl) type.newInstance();
}


@After public void cleanup() {
    Thread.currentThread().setUncaughtExceptionHandler(null);
}

kevinrushforth · 2020-04-14T11:46:47Z

/reviewers 2

openjdk · 2020-04-14T11:46:56Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth · 2020-04-14T11:47:21Z

@koppor are you planning to resume work on this PR?

kevinrushforth · 2020-05-08T18:49:05Z

Closing, since there has been no activity for 3 months.

@koppor - if you would like to pursue this, please address the pending feedback and reopen the PR.

8176270: Fix TextInputControl String boundaries

492cb6c

bridgekeeper bot added the oca Needs verification of OCA signatory status label Dec 21, 2019

koppor changed the title ~~dding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException~~ 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException Dec 21, 2019

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Dec 21, 2019

koppor mentioned this pull request Dec 21, 2019

Fixes StringIndexOutOfBoundsException when cutting text JabRef/jabref#5776

Merged

5 tasks

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Feb 7, 2020

openjdk bot added the rfr Ready for review label Feb 7, 2020

kevinrushforth requested changes Feb 7, 2020

View reviewed changes

koppor mentioned this pull request Feb 12, 2020

Cutting some of the bibtexkey raises error: Bale13 -> Bal13 JabRef/jabref#5708

Closed

1 task

kleopatra mentioned this pull request Mar 5, 2020

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #138

Closed

3 tasks

kevinrushforth closed this May 8, 2020

koppor deleted the fix-JDK-8176270 branch April 10, 2022 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

koppor commented Dec 21, 2019 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 21, 2019

koppor commented Dec 21, 2019

bridgekeeper bot commented Dec 21, 2019

mlbridge bot commented Feb 7, 2020

kevinrushforth left a comment

kevinrushforth Feb 7, 2020

kleopatra Feb 12, 2020

kevinrushforth Feb 7, 2020 •

edited

Loading

kleopatra Feb 9, 2020

kevinrushforth Feb 7, 2020

kleopatra commented Feb 9, 2020 •

edited

Loading

kevinrushforth commented Apr 14, 2020

openjdk bot commented Apr 14, 2020

kevinrushforth commented Apr 14, 2020

kevinrushforth commented May 8, 2020

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

Conversation

koppor commented Dec 21, 2019 • edited by openjdk bot Loading

Progress

Issue

Download

bridgekeeper bot commented Dec 21, 2019

koppor commented Dec 21, 2019

bridgekeeper bot commented Dec 21, 2019

mlbridge bot commented Feb 7, 2020

Webrevs

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Feb 7, 2020

Choose a reason for hiding this comment

kleopatra Feb 12, 2020

Choose a reason for hiding this comment

kevinrushforth Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

kleopatra Feb 9, 2020

Choose a reason for hiding this comment

kevinrushforth Feb 7, 2020

Choose a reason for hiding this comment

kleopatra commented Feb 9, 2020 • edited Loading

kevinrushforth commented Apr 14, 2020

openjdk bot commented Apr 14, 2020

kevinrushforth commented Apr 14, 2020

kevinrushforth commented May 8, 2020

koppor commented Dec 21, 2019 •

edited by openjdk bot

Loading

kevinrushforth Feb 7, 2020 •

edited

Loading

kleopatra commented Feb 9, 2020 •

edited

Loading